feat: separate flags for request/response E2E checksum and enable request checksum by default #1251
+239
−52
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces the
GOOGLE_CLOUD_DATASTORE_HTTP_ENABLE_E2E_CHECKSUM
flag with two:GOOGLE_CLOUD_DATASTORE_HTTP_ENABLE_E2E_REQUEST_CHECKSUM
- attach payload checksum header to requests (enabled by default)GOOGLE_CLOUD_DATASTORE_HTTP_ENABLE_E2E_RESPONSE_CHECKSUM
- verify response checksum (if extsts in response headers) against response payload and raise IOException if it doesn't match (disabled by default)And enables the E2E request checksum by default.
This is safe because nobody is using the old flag and we don't reject calls with invalid checksum on the server side yet. But this will provide us the analytics of number of requests with valid/invalid checksums to find edge cases and better prepare for E2E checksum rollout.
The tests for
RemoteRpc
using request and response checksum was also reworked.Previously the test called the protected
setHeaders
but it does not verify thatRemoteRpc
will call it when making a call. I. e., if in the future we changeRemoteRpc
'scall
method and remake adding the headers without using thesetHeaders
method then the tests will remain green while necessary headers may not be added.Instead of calling
setHeaders
from the test itself, we pass the expected headers to theMyHttpTransport
and make sure the required headers are in place whenMyLowLevelHttpRequest
is doing itsexecute
call.It is easy to convert the remaining test (not related to E2E checksum) that uses
setHeaders
to the same approach, then we can makesetHeaders
private. I can do it in a separate PR if you like.Another change in the test is that now we verify that checksum in the header is generated correctly (not just checking the length of the checksum string) and verify that the response checksum gets validated.